Skip to content

Fix NDE caused by removing Workflow.getVersion with a succeeding Work… #2370

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Merged
merged 2 commits into from
Jan 16, 2025

Conversation

Quinn-With-Two-Ns
Copy link
Contributor

closes #2367

@Quinn-With-Two-Ns Quinn-With-Two-Ns requested a review from a team as a code owner January 15, 2025 19:48
Copy link
Member

@cretz cretz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I kinda understand this, but I won't pretend to deeply understand it (I don't understand very deeply why Java chose to do fake commands for matching), but given the fact that nobody but you might be able to understand it without significant effort, I'm approving.

@@ -525,6 +525,24 @@ private void handleCommandEvent(HistoryEvent event) {
continue;
}

if (VersionMarkerUtils.hasVersionMarkerStructure(event)
&& !VersionMarkerUtils.hasVersionMarkerStructure(command.getCommand())) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I understand what's happening here. So is !VersionMarkerUtils.hasVersionMarkerStructure(command.getCommand()) basically saying isFakeVersionMarker?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah this code deserves a comment. I will add it. !VersionMarkerUtils.hasVersionMarkerStructure(command.getCommand()) is basically checking if the command is from a Workflow.getVersion call. What I am checking here is, if the history event is a version marker, but the command isn't that means this event can't match with the command so it must be a removed version marker

Copy link
Member

@Sushisource Sushisource left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense to me

@Quinn-With-Two-Ns Quinn-With-Two-Ns merged commit 90e5125 into temporalio:master Jan 16, 2025
8 checks passed
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Removing a Workflow.getVersion with a succeeding Workflow.sideEffect causes NDE
3 participants